bpo-34969: Add --fast, --best on the gzip CLI by matrixise · Pull Request #9833 · python/cpython

matrixise · 2018-10-13T05:48:56Z

https://bugs.python.org/issue34969

JulienPalard · 2018-10-13T09:07:12Z

I'm not for implementing -1 and -9 without implementing the whole range. Why not removing them, keeping only --best and --fast?

matrixise · 2018-10-13T10:37:05Z

good catch, I have removed -1, -9 and -d can't accept --fast and --best.

tirkarthi · 2018-10-13T12:03:38Z

Minor nit, I am not sure bester is a valid word. "Best compression" maybe?

JulienPalard

If you change the default compression from the CLI you may want to tell it in the NEWS file.

JulienPalard · 2018-10-13T14:04:56Z

Missing dot at end of sentence.

JulienPalard · 2018-10-13T14:05:22Z

Same missing dot.

bedevere-bot · 2018-10-13T14:13:09Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vadmium · 2018-10-14T04:35:28Z

Out of date?

@vadmium sure, if we use the level 6, it's not the --best flag.

thanks

sure, thanks

vstinner · 2018-10-19T21:35:30Z

"unversionned" python must not be used: please write python3 (and change the existing documentation for "file").

I'm not sure that it's useful to add an example of a command for each command line option.

Actually, most examples in the current Doc do use an unversioned python for -m examples. Only a few currently use python3 -m. We should probably decide on which is better and fix them all. But that should be a separate issue and PR.

I think it's better to use pythoninstead of python3 since that will add the extra work of replacing all python3 with python4 when we have a release in future :) Many examples will be compatible with both versions and also might cause issues in backporting doc fixes between two major versions . The URL and drop-down at the top already indicates python3.X version. I might be missing a use case here.

I remove the example with the command line and in this way, just avoid the "un.versioned" python.

vstinner · 2018-10-19T21:37:59Z

Please make these constants private. If you really want to make them public, you should document them.

maybe with a new PR/bpo

bedevere-bot · 2018-10-19T21:38:18Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2018-10-25T08:38:55Z

this test is redundant, assert_python_ok() ensures thta it's 0.

vstinner · 2018-10-25T08:41:49Z

11 lines just to pass a flag? IMHO it's too much, it can be done with less code: see below.

vstinner · 2018-10-25T08:46:28Z

"gzip: Add --fast and --best options on the gzip CLI ..."

vstinner · 2018-10-25T08:48:14Z

I suggest to remove "it's a good tradeoff between the best and fast compression methods". I wouldn't promise that it's a good tradeoff. Compression is full of bad surprises.

Just one example from gzip manual page:
"In some rare cases, the --best option gives worse compression than the default compression level (-6). On some highly redundant files, compress compresses better than gzip."

vstinner · 2018-10-25T08:53:12Z

Can't we expect a specific exit code? Same question for following test.

--fast and --best are the new parameters. They indicate to Gzip if we want to have a fast compression method or a slow method (best compression).

… -3, -4, etc...

gzip.

vstinner

LGTM.

@JulienPalard: Can you please double please? You can merge it if it looks good to you.

matrixise · 2018-11-03T11:56:50Z

@JulienPalard are you interested by a merge of this PR?

JulienPalard · 2018-11-03T15:23:56Z

LGTM too :) thanks @matrixise for this PR!

the-knights-who-say-ni added the CLA signed label Oct 13, 2018

bedevere-bot added the awaiting review label Oct 13, 2018

matrixise force-pushed the bpo-34969 branch from df7720c to 54d1131 Compare October 13, 2018 10:36

tirkarthi reviewed Oct 13, 2018

View reviewed changes

Comment thread Lib/gzip.py Outdated

tirkarthi Oct 13, 2018

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, I am not sure bester is a valid word. "Best compression" maybe?

JulienPalard requested changes Oct 13, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Oct 13, 2018

JulienPalard approved these changes Oct 13, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 13, 2018

vadmium reviewed Oct 14, 2018

View reviewed changes

vstinner requested changes Oct 19, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Oct 19, 2018

matrixise force-pushed the bpo-34969 branch from 841ec86 to 893da81 Compare October 20, 2018 09:56

vstinner requested changes Oct 25, 2018

View reviewed changes

matrixise added 10 commits October 25, 2018 14:22

Add two mutually exclusive parameters for the command line of gzip.

0cfe350

--fast and --best are the new parameters. They indicate to Gzip if we want to have a fast compression method or a slow method (best compression).

Add the blurb entry with the bpo number

a0d6e0c

make patchcheck

4347fc8

by default, the compression level will be 6 (tradeoff), and not 9 (best)

6fde313

does not allow the compresslevel for the decompress method

737737c

remove the options '-1', '-9' to avoid a future implementation of -2,…

6b2c0f2

… -3, -4, etc...

Fix typo, bester -> better

2867cfb

Update with the recommendations of Julien

8b05c89

Remove the examples from the documentation for the CLI of the module

f38577a

gzip.

Use private constants

7bcc2f1

matrixise added 3 commits October 25, 2018 14:22

Remove add_compress_level_flag helper in the test of gzip

6547cbe

Add the module name as prefix of the blurb entry

74647af

Reword the sentence about the default value for the compression level

fdaa34b

matrixise force-pushed the bpo-34969 branch from 893da81 to fdaa34b Compare October 25, 2018 12:26

remove useless tests about rc==0

6e67e72

vstinner approved these changes Oct 25, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 25, 2018

JulienPalard merged commit 3e28eed into python:master Nov 3, 2018

bedevere-bot removed the awaiting merge label Nov 3, 2018

wchargin mentioned this pull request Jan 26, 2020

[3.7] bpo-39389: gzip: fix compression level metadata (GH-18077) #18101

Merged

Uh oh!

Uh oh!

Conversation

matrixise commented Oct 13, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JulienPalard commented Oct 13, 2018

Uh oh!

matrixise commented Oct 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JulienPalard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

matrixise commented Nov 3, 2018

Uh oh!

JulienPalard commented Nov 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

matrixise commented Oct 13, 2018 •

edited by bedevere-bot

Loading